-
-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #4138 fix delivery address #4540
base: main
Are you sure you want to change the base?
Conversation
* group examples together with variables * rubocop -A passes on this file without needing to disable ArrayAlignment
* Compare generated against expected PDFs * Add helper test to regenerate expected PDFs * Add helper module
…ethod * Address output prints delivery address if filled in, otherwise partner address * Only does this for delivery/shipped method
Hey @jimmyli97 As a matter of process, in the future please get yourself assigned to the issues when you are going to be working on them - or at least comment on them. This helps us avoid duplication of effort. Thanks. |
@cielf it automatically unassigned me, should I keep commenting every 7 days? |
Oops - sorry, my bad (I somehow didn't see that you had been assigned and weren't anymore). Not to worry. |
But if it's going to lapse while you are still working on it, yeah - commenting makes sense -- otherwise someone else may pick it up. Once you've put in the pull request, it's not as big a deal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm. I tried blanking out the two addresses, and got a mess. This shouldn't happen in real life (if it's being delivered, there really should be a delivery address, but I could see it happening in a situation where the bank is very familiar with a new partner, and some staff drops by the partner with the goods on their way home or something ). I'd still like to see it fixed. (You could suppress the "Delivery address" label in this case too.)
* REFACTOR out profile name and email
My concerns have been addressed. Over to @dorner for a technical look-see.
Hey @dorner -- it looks like the ball on this one is currently in your court? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks OK - I had some concerns with the skipped helper method.
.gitignore
Outdated
@@ -49,6 +49,7 @@ dump.rdb | |||
.DS_Store | |||
.ruby-gemset | |||
!spec/fixtures/files/*.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you adding this if you've added these files? Shouldn't they be tracked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 51 tells git to ignore all .pdf files so line 52 specifies an exception (by prefixing an exclamation mark) to track the pdf files in spec/fixtures/files
added a comment to clarify in 0d53723
spec/pdfs/distribution_pdf_spec.rb
Outdated
compare_pdf(create_dist(:pick_up), expected_pickup_file) | ||
end | ||
end | ||
# this test is a helper function to regenerate expected PDFs, only commit with it skipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really confusing. If you want a helper method, why not define it as a method, and people can run it from the Rails console?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to generate the pdf files correctly, the helper method generating the file needs to have the same test environment setup as the actual test itself. This way you don't have to repeat code setting up the test environment. I did research but I could not figure out a better way to do it without duplicating code.
you can call this method from the terminal by calling the test's line number e.g. bundle exec rspec spec/pdfs/distribution_pdf_spec.rb:227
.
I added a comment clarifying this in 0d53723
alternatively I could delete the method if you want, but I found it super helpful because I could just run that line whenever I made changes to the pdf generation and regenerate the test files. so if anyone in the future makes changes they could do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should definitely be a way to do this without duplicating code. Maybe it's the setup that needs to be extracted to a base method that's called from both places. And worse comes to worst, if you do need to duplicate a few lines of code, it's still a lot less messy than a test that's not actually a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with a solution which checks for an environment variable and if it's set to true to overwrite the expected PDFs in 4516951 . I think it's a lot cleaner, let me know if that works.
* Use environment variable for regenerating comparison pdfs
@jimmyli97 FYI: We had an urgent fix that required all the senior contributors this week , so we didn't get to look at this again. Hopefully this week will go better. |
@jimmyli97 this helps a bit but not by much. Essentially:
That does not imply that in order to generate the PDF, you should run an RSpec. It means you should extract the behavior that does the environment setup to shared code, and run that setup code both in the RSpec and in a separate script that can be used to generate the PDF. |
922afc6
to
26ece6e
Compare
@dorner I managed to figure out a way to separate out the setup code so you can call a Rails console method which calls that setup code, and also the Rspecs call that setup code. it's a bit messy though, let me know what you think |
spec/pdfs/distribution_pdf_spec.rb
Outdated
let(:item3) { create(:item, name: "Item 3", value_in_cents: 300) } | ||
let(:item4) { create(:item, name: "Item 4", package_size: 25, value_in_cents: 400) } | ||
before(:each) do | ||
@organization, @storage_location, @item1, @item2, @item3, @item4 = create_organization_storage_items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to do this via instance variables. Here's how I'd do it to keep the RSpec looking the same:
let(:storage_creation) { create_organization_storage_items }
let(:organization) { storage_creation.organization }
let(:item1) { storage_creation.items[0] }
# etc.
See below where you can change this method to make it easier to work with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 6da7702
item3 = FactoryBot.create(:item, name: "Item 3", value_in_cents: 300) | ||
item4 = FactoryBot.create(:item, name: "Item 4", package_size: 25, value_in_cents: 400) | ||
|
||
[org, storage_location, item1, item2, item3, item4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd define a struct above, something like
StorageCreation = Data.define(:organization, :storage_location, :items)
and here you could return
StorageCreation.new(org, storage_location, [item1, item2, item3, item4])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 6da7702
private def create_profile(program_address1, program_address2, program_city, program_state, program_zip) | ||
create(:partner_profile, | ||
def create_organization_storage_items | ||
org = FactoryBot.create(:organization, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this setup is being used outside RSpec, it shouldn't be in spec/support
and it shouldn't use FactoryBot. It should work more like the seeds file where you create data manually. Probably should live in its own folder under the /lib
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in ea7e241
organization: organization) | ||
end | ||
|
||
def create_file_paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you actually creating file paths here? Or just returning the ones you have specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to get_file_paths
in 6da7702
10e5b18
to
ea7e241
Compare
failing test is a known flaky one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ete item units correctly when generating test files
fixed in 39e8cb4 |
(6,1) failing test is known flake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're very, very close!
If there is no partner primary contact info, but there is a delivery address, you get something like this:
I checked production to see if this happens in the wild -- there are about 400 partners with this situation -- probably because the primary contact info wasn't there from day 1.
… blank * Add rspec * Other comparison pdfs change because previously blank phone # skipped line, now blank phone # adds a blank line * Move instance_method call inside function
Better! It does feel to me like there is a bit too much space between the Issued To and the Delivery Address, though. Is it possible to tighten that up without side effects? |
fixed spacing in b00d1b0 |
Functionality works great! Thank you! Back to @dorner for final tech review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Had some suggestions around the helpers (I know, this has been going around forever, but I think one last round should do it).
|
||
private def create_comparison_pdf(storage_creation, profile_create_method, expected_file_path, delivery_method) | ||
partner = create_partner(storage_creation.organization) | ||
profile = PDFComparisonTestFactory.instance_method(profile_create_method).bind_call(Class.new.extend(PDFComparisonTestFactory), partner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit over-the-top... can we not just move the possible inputs to this to class methods? Then you can just call PDFComparisonTestFactory.send(profile_create_method)
.
profile.destroy! | ||
dist.request.destroy! | ||
dist.destroy! | ||
partner.destroy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to just wrap this in a transaction and roll it back (i.e. raise an ActiveRecord::Rollback
) rather than manually calling destroy
.
storage_creation.items[2].destroy! | ||
storage_creation.items[3].request_units.destroy_all | ||
storage_creation.items[3].destroy! | ||
storage_creation.organization.destroy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the transaction/rollback can be moved to this method.
|
||
RSpec.configure do |c| | ||
c.include DistributionPDFHelper | ||
c.include PDFComparisonTestFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These shouldn't need to be included in the base RSpec config. I generally prefer moving everything to module/class methods instead of instance methods. There aren't any instance variables, so doing that makes everything way easier to work with - so you can just call PDFComparisonTestFactory.create_comparison_pdfs
directly, and don't have to pollute existing specs or configs.
Resolves #4138
Description
Prints no address if delivery_method is pick_up
Prints partner address if partner has no program address and delivery_method is delivery or shipped
Prints partner program address if partner has program address and delivery_method is delivery or shipped
Tests output against expected PDFs
Adds helper test to generate expected PDFs
Type of change
How Has This Been Tested?
Passes test suite
Manual testing